Fix reimporting assets with csv in the project #92320
Conversation
AThousandShips
added
the
cherrypick:4.2
Hilderin
force-pushed
the
fix-importing-assets-with-csv
branch
from
4e18c87
to
9ad1d4d
Compare
editor/editor_file_system.cpp
Outdated
|
|
||
| // We need to increment the counter, maybe the next file is multithreaded | ||
| // and don't have the same importer. | ||
| from = i + 1; |
There was a problem hiding this comment.
This part seems related to #85130, which I guess aims to address the same issue (albeit in another branch).
I couldn't get full confidence there that the change is correct. Would be good to re-review both PRs conjointly and figure out what this code is trying to do.
There was a problem hiding this comment.
My first tentative was exactly what was done in #85130 but the problem is that the variable 'from' is used elsewere after the Ref<ResourceImporter> importer = ResourceFormatImporter::get_singleton()->get_importer_by_name(reimport_files[from].importer);.
That is how I see it...
In my case I had:
Index 0: threaded = false, importer = csv_translation
Index 1: threaded = true, importer = texture
Index 2: threaded = true, importer = texture
Index 3: threaded = true, importer = texture
Index 4: threaded = false, importer = scene
If I use reimport_files[i].importer :
The first iteration, i = 0 and from = 0, _reimport_file was called directly
The second iteration, i = 1 and from = 0, it tries to start the thread because the importers of index 0 (from) and 2 (i+1) are different which is not was expected:
The thread will be started with the wrong index:
Edited: I looked more closely the comments on the #85130 from @RandomShaper and effectively, there is another potential problem that I did not saw when !importer.is_valid().
I'll look into it.
There was a problem hiding this comment.
This have been addressed in this commit: 4f55118
I should have been clearer in my comment below.
Hilderin
force-pushed
the
fix-importing-assets-with-csv
branch
from
9ad1d4d
to
4f55118
Compare
|
This last commit included modifications to fix #85130 and the comments from @RandomShaper |
|
I think this could fix at least partially the issue #58131 With the fix, the preview are wrong because they are based on .import files and .godot folder but the texture are working fine: |
Hilderin
force-pushed
the
fix-importing-assets-with-csv
branch
from
4f55118
to
63ccccb
Compare
Hilderin
force-pushed
the
fix-importing-assets-with-csv
branch
from
63ccccb
to
760e20f
Compare
Hilderin
force-pushed
the
fix-importing-assets-with-csv
branch
from
760e20f
to
bce48a9
Compare
|
Sorry, @AThousandShips I'm not sure what you are referencing with:
Everything should be good, I think. |
Hilderin
force-pushed
the
fix-importing-assets-with-csv
branch
from
bce48a9
to
f1099ab
Compare
|
Welp I merged this inadvertently because I was testing it in my But I guess it's a good way to confirm whether it's working or not, let's see how it fares from now to when I'd need to make beta3 builds. Thanks! |
This should fix #83200
The problem was that the
DebugDialogue.csv.importis invalid according to Godot because it’s missing thepathline. Because of this the importer was not mark multithread and a counter was not incremented between threaded and non threaded reimportation. I fixed the increment problem ineditor_file_system._reimport_file.The problem with the invalid
.import filewas caused becauseResourceImporterCSVTranslationis the only importer that has no extension. It does not write any file in the .godot folder. I fixed that adding a check in_get_path_and_typebefore returning ERR_FILE_CORRUPT.